Skip to content

Configurable doubleclick#3991

Merged
etpinard merged 11 commits into
plotly:masterfrom
pynklu:2206-configurable-double-click-delay
Jul 22, 2019
Merged

Configurable doubleclick#3991
etpinard merged 11 commits into
plotly:masterfrom
pynklu:2206-configurable-double-click-delay

Conversation

@pynklu

@pynklu pynklu commented Jun 27, 2019

Copy link
Copy Markdown

Add an option 'doubleClickDelay' to the layout object (0 - 1000ms), to optionally overwrite the default 300ms doubleclick delay
#2206

first time making a PR (and don't really work with git), so sorry in advance if i did something wrong :-)

@etpinard

etpinard commented Jun 27, 2019

Copy link
Copy Markdown
Contributor

Thanks very much for the PR!

I think doubleClickDelay would be better off as config option, unless @plotly/plotly_js opposes.

We'll also need to make all subplots including the ones that don't use dragElement (e.g. geo and mapbox), use the set double-click value.

Thanks again, let me know if you any other questions.

@pynklu

pynklu commented Jun 28, 2019

Copy link
Copy Markdown
Author

Ah yes, that makes more sense. Can I add extra commits to this PR, or?

@pynklu

pynklu commented Jun 28, 2019

Copy link
Copy Markdown
Author

Alright, I changed it to use the config object. I went through the geo & mapbox code & couldn't find anything doublclick related that doesn't come from dragelem/ or legend/
Couldn't find any double-click behaviour that didn't use the adjusted delay either through manual testing

@etpinard

etpinard commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

I went through the geo & mapbox code & couldn't find anything doublclick related that doesn't come from dragelem/ or legend/

See

plotly.js/src/plots/geo/geo.js

Line 436 in 3d383b7

bgRect.on('dblclick.zoom', zoomReset);

which uses the d3 double click handler

plotly.js/src/plots/mapbox/mapbox.js

Lines 193 to 211 in 3d383b7

map.on('dblclick', function() {
var optsNow = gd._fullLayout[self.id];
Registry.call('_storeDirectGUIEdit', gd.layout, gd._fullLayout._preGUI, self.getViewEdits(optsNow));
var viewInitial = self.viewInitial;
map.setCenter(convertCenter(viewInitial.center));
map.setZoom(viewInitial.zoom);
map.setBearing(viewInitial.bearing);
map.setPitch(viewInitial.pitch);
var viewNow = self.getView();
optsNow._input.center = optsNow.center = viewNow.center;
optsNow._input.zoom = optsNow.zoom = viewNow.zoom;
optsNow._input.bearing = optsNow.bearing = viewNow.bearing;
optsNow._input.pitch = optsNow.pitch = viewNow.pitch;
gd.emit('plotly_doubleclick', null);
gd.emit('plotly_relayout', self.getViewEdits(viewNow));
});

which uses mapbox-gl's double click handler

There might be a way to make those two handlers adhere to gd._context.doubleClickDelay, but I'd be ok punting these to #2057

@etpinard

etpinard commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

Now @pynklu would you be interested in trying to:

  • make npm run test-jasmine --bundleTest=requirejs_test.js --nowatch pass
  • add a new test similar to this one in the click_test.js suite

@pynklu

pynklu commented Jul 2, 2019

Copy link
Copy Markdown
Author

afbeelding
doesn't this imply that test passes?

@etpinard

etpinard commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

this here

image

is the failing test (ref: https://circleci.com/gh/plotly/plotly.js/43400?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link)

@pynklu

pynklu commented Jul 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

@etpinard sorry, but I can't get the tests to run properly locally on my end + really don't know how to properly write new tests. Would you mind taking this up? :-)
Let me know if there's anything else I can help out with

@etpinard

etpinard commented Jul 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

TODO (that hopefully I'll have time to do before 1.49.0 is ready):

  • add 1 test in config_test.js, not needed as the logic is trivial
  • add a few dragelement_test.js tests
  • add a few legend tests
  • add note in doubleClickDelay plot_config.js description about limitations i.e. it has no effect on geo and mapbox subplot double-click delays for now.

@etpinard etpinard added this to the v1.49.0 milestone Jul 22, 2019
@etpinard

etpinard commented Jul 22, 2019

Copy link
Copy Markdown
Contributor

cc @plotly/plotly_js this PR is now rewiewable and tagged under 1.49.0 (why not right?) - I'm looking for a review today. Thanks!

@archmoj

archmoj commented Jul 22, 2019

Copy link
Copy Markdown
Contributor

Thanks @pynklu & @etpinard.
Nicely done.
💃

@etpinard
etpinard merged commit 84e79e4 into plotly:master Jul 22, 2019
@pynklu
pynklu deleted the 2206-configurable-double-click-delay branch August 27, 2019 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

feature something new

Projects

None yet

Milestone

v1.49.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pynklu @etpinard @archmoj